Could we cut down on the comment size here to make this more concise?

Addressed in cc1a135: comment now captures the operational why only. The full attack chain stays in the PR body and tests.

It looks like O_NOFOLLOW is not supported on windows.

You can see in a similar vulnerability in a filelock python package here took a Windows specific patch in the "Patches" section.

If you're able we'd very much appreciate an investigation into the windows specific fix as well! But in the meantime, could you document this fix only applies to Posix? We can do a check for constants.O_NOFOLLOW !== 0 and make a decision about whether we support --auth-file as well here 🤔 cc @danhill-stripe

Addressed in cc1a135: flag is now constants.O_NOFOLLOW ?? 0 with an inline note on the Windows gap (O_EXCL still prevents overwriting a pre-existing entry, but no full no-follow semantics there). The proposed Windows fix and the plan to validate it on Windows CI before promising it are in the top-level comment.

Sorry one more q - in what context would this be triggered and how has its behavior changed? Why would we want to unlink a symlinked file? Before fs.access was just testing access permissions - why do we want to now potentially unlink? Wouldn't that invalidate the checks below about not following symlinked files?

Thanks for the close read. Fair point that the prior --force flow didn't fully match the no-follow contract.

The unlink runs only when --force is set, and it clears the path so the subsequent open with O_CREAT | O_EXCL can succeed. For regular files that preserves the prior --force semantics (replace the existing output file), with the credential landing in a fresh 0o600 inode rather than racing a chmod on an already-open file descriptor.

For symlinks specifically, the no-follow guarantee doesn't cover the existing entry. The unlink runs before O_NOFOLLOW can fire, so the protection only covers the race window after the unlink, not the symlink that was already there. The credential never reaches the link target, but --force ends up silently destroying a symlink the operator may not have intended to remove.

Pushed a fix: lstat first, refuse if the final path is a symlink (with or without --force), unlink and recreate only for non-symlink existing files. O_EXCL | O_NOFOLLOW stays as the race defense between the precheck and the atomic create. With this, --force replaces an existing regular output file and rejects everything else.

Thanks again for the careful pass.

The first part of the change makes sense. Why don't we simply add that check up top and leave the rest of the code unchanged? I'm lost on why we need all the changes below. I'd propose we keep things simply

let existing: Stats | undefined;
  try {
    existing = await fs.lstat(resolved);
  } catch (err) {
    if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err;
  }

  if (existing?.isSymbolicLink()) {
    throw new Error(
      `OUTPUT_FILE_SYMLINK: ${resolved} is a symbolic link. Refusing to write as symbolic links can open a TOCTOU security threat.`,
    );
  }

if (!force) {
    try {
      await fs.access(resolved);
      throw new Error(
        `OUTPUT_FILE_EXISTS: ${resolved} already exists. Use --force to overwrite.`,
      );
    } catch (err) {
      if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err;
    }
  }

  await fs.writeFile(resolved, JSON.stringify(data, null, 2), {
    mode: 0o600,
  });
  await fs.chmod(resolved, 0o600);
  return resolved;

That would make the diff much more reasonable.

Yep, I agree the current diff is larger than ideal.

The part I'm hesitant to drop is the final open path. The lstat check up front is useful for clearer errors, but it is still only a precheck. If we go back to fs.writeFile, the actual write resolves the path again and follows symlinks.

The other subtle bit is mode: 0o600: it only applies when a new file is created. In the --force path, if the destination already exists, writeFile writes first and the explicit chmod happens after the credential bytes are already on disk.

So I think we need two things:

1. Reject an existing symlink at the output path.
2. Make the actual credential write create a fresh 0o600 file atomically.

Your suggested lstat block covers the first one. The fs.open(O_CREAT | O_EXCL | O_WRONLY | O_NOFOLLOW, 0o600) part is what covers the second one.

A concrete --force race I'm trying to avoid:

T0: we lstat and see no file at resolved
T1: another local user creates resolved as a symlink to a file they can already read
T2: because force=true, the fs.access check is skipped
T3: fs.writeFile follows the symlink and writes the credential into that target
T4: fs.chmod follows the symlink and locks the target down to 0o600, but the credential was already written through the symlink

I can definitely trim the implementation and comments so the diff is smaller. My proposal would be:

• keep your lstat precheck for clear symlink errors
• keep --force limited to replacing existing regular files
• keep the final write as atomic fs.open(... O_EXCL | O_NOFOLLOW, 0o600) plus handle.write

That should keep the PR focused while preserving the part that closes the TOCTOU.